-
-
Notifications
You must be signed in to change notification settings - Fork 364
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: pass standard params to storage #791
Conversation
f0f4964
to
c8e469d
Compare
@@ -159,10 +159,12 @@ func (a *Request) Merge(request Requester) { | |||
} | |||
} | |||
|
|||
var defaultAllowedParameters = []string{"grant_type", "response_type", "scope", "client_id"} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my own code, I use the following list:
"max_age", "prompt", "acr_values", "id_token_hint", "nonce",
// PCRE.
"code_challenge", "code_challenge_method",
// Other fields.
"display", "ui_locales", "login_hint",
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW, this is not really a "default" list (you cannot set it to non-default) but an "always allowed list".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW, this is not really a "default" list (you cannot set it to non-default) but an "always allowed list".
True :)
I would probably drop id_token_hint
because it contains a valid ID token
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, thanks. You are right. I am storing that to support logout. But I should just store the subject ID, not the whole token.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In fact, id_token_hint
is from this list in fosite: https://github.com/ory/fosite/blob/master/handler/openid/flow_explicit_auth.go#L29-L35
So if you are saying this should not be stored into the database? Because fosite currently does so:
if err := c.OpenIDConnectRequestStorage.CreateOpenIDConnectSession(ctx, resp.GetCode(), ar.Sanitize(oidcParameters)); err != nil {
return errorsx.WithStack(fosite.ErrServerError.WithWrap(err).WithDebug(err.Error()))
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this means that subject should be extracted out earlier from the id_token_hint
instead of being passed to the store inside form?
Related Issue or Design Document
Checklist
If this pull request addresses a security vulnerability,
I confirm that I got approval (please contact security@ory.sh) from the maintainers to push the changes.
Further comments